fix(mcp): resolve superseded failed mutation receipts on later success - #693

Thanks for this — the diagnosis in #692 and the follow-up correction are the good kind of bug report, and the core of this fix is right. I verified the load-bearing argument independently rather than reading it: a completed waiter always has RequestedGeneration <= AppliedGeneration, because completeMutationWaiters only completes waiters whose generation <= appliedGeneration (watcher.go:1774, and the storm path at :2042). So a success at requested generation G guarantees the graph reflects at least G's bytes, hence strictly newer than any failure below G — and thresholding on the requested rather than the applied generation is the conservative side of that. The claim holds.

Build, go vet, golangci-lint ./internal/mcp/... clean. The full ./internal/mcp suite passes here on macOS, and again under -race (198s, no data races) — including TestDetectChanges_EchoesMeasuredScope and TestFileMutationsReindexSynchronouslyAcrossConsecutiveEdits, which you saw fail in your Linux container. So those look environmental rather than pre-existing-and-known; worth a second look on your side, but not this PR's problem.

Mutants: removing the resolveSupersededFailedReceipts call fails TestTrackMutationTicketResolvesSupersededFailures, dropping the generation guard fails TestMutationFreshnessSuccessResolvesSupersededFailures, and dropping the path guard fails it too. Three of the four guards bind. The fourth is finding 2 below.

One regression I need fixed before this lands, and one gap.

1. change.receipt reports a superseded failure as pending, permanently

Deleting the receipt removes the only thing that could refresh the mutation-commit ledger's graph half. mutationStatusPayload refreshes a record's status via mutationReceiptState(record.pendingReindexReceipt()) and simply skips the refresh when the lookup misses — so a record whose edit response timed out stays graph_status: "pending" forever once its receipt is dropped.

Same probe, natural sequence (edit response times out → record is pending with receipt mutation-3; the ingest fails terminally; nobody polls in that window; a later generation of the same path succeeds), asserting only what change.receipt would print:

origin/main   after supersede   graph_status="failed"
this branch   after supersede   graph_status="pending"

failed is at least true of that generation and actionable. pending is neither — and mutationStatusGuidance for a committed write says "if graph_status is not "fresh" the graph is still catching up", so the caller is told to keep waiting for a generation that will never complete. That is exactly the failure mutationStatusPayload's own doc comment says the refresh exists to prevent ("every pending receipt would read as pending forever, which is precisely the stale answer this tool exists to avoid").

The window is not exotic — this PR widens it. Today an agent learns about the failed ingest from the detect refusal and then goes and reads the receipt. After this change the refusal is gone, so there is less reason to poll in the interval where the status is still truthful.

Worth contrasting with the batch path, which already models a vanishing receipt correctly: refreshBatchGraph clears file.ReindexReceipt on a lookup miss and re-admits the file (batch_transaction.go:909-922). mutationStatusPayload has no such fallback.

Suggested fix — don't delete, resolve. Stamp the superseding outcome onto the failed receipt instead of removing it. Prototyped locally; it is a few lines in place of the Delete:

other.mu.Lock()
if other.completed && (other.result.Err != nil || !other.result.Reindexed) {
    other.result = indexer.MutationResult{
        RequestedGeneration: other.generation,
        AppliedGeneration:   applied.AppliedGeneration,
        Reindexed:           true,
    }
}
other.mu.Unlock()

With that, the barrier clears exactly as it does now (I ran a barrier assertion against both variants — both pass), the receipt stays queryable, and change.receipt reports:

prototype     after supersede   graph_status="fresh"   (reindexed=true, applied=9)

which is not just better than this branch, it is better than main — the path genuinely is fresh once a later generation lands, and failed was already the wrong answer there. It also mirrors what the watcher itself does: completeMutationWaiters resolves every earlier waiter with the later apply's result rather than discarding it, so the MCP ledger would end up describing supersession the same way the layer beneath it does.

One caveat if you take this: read the succeeded result before the Range and pass it in (you already have result in hand at the call site in trackMutationTicket) rather than taking succeeded.mu.RLock() inside the loop while holding nothing — two concurrent resolves each holding their own read lock and reaching for the other's write lock is a deadlock shape that does not need to exist.

2. The "pending receipts are untouched" guard is the one that isn't tested

terminalFailure := true — i.e. delete every earlier-generation receipt for the path, in flight or not — leaves the entire ./internal/mcp suite green. Every receipt in TestMutationFreshnessSuccessResolvesSupersededFailures is already completed, so nothing exercises it.

That is the guard whose failure direction is unsafe: dropping a pending receipt removes a live freshness gap from the barrier rather than a dead one. Test that fails under that mutant and passes at head:

func TestMutationFreshnessSuccessKeepsPendingSamePathReceipts(t *testing.T) {
	s := &Server{mutationSafetyWait: time.Millisecond}
	pendingFreshnessReceipt(s, "receipt-inflight", "repo-a", "/repo-a/file.go", 4)

	succeeded := pendingFreshnessReceipt(s, "receipt-success", "repo-a", "/repo-a/file.go", 9)
	completeFreshnessReceipt(succeeded, indexer.MutationResult{
		RequestedGeneration: 9, AppliedGeneration: 9, Reindexed: true,
	})
	s.resolveSupersededFailedReceipts(succeeded)

	if _, loaded := s.mutationReceipts.Load("receipt-inflight"); !loaded {
		t.Fatal("a still-pending receipt for the same path was dropped by the resolve")
	}
	err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a")
	if err == nil || !strings.Contains(err.Error(), "receipt-inflight") {
		t.Fatalf("barrier no longer reports the in-flight receipt: %v", err)
	}
}

(Adjust the receipt-identity assertion if you take the resolve-in-place route.)

Scope, so #692 does not get closed by accident

trackMutationTicket is reached only from mutationReindexState and the batch transaction. reindex_repository goes through multiIndexer.IncrementalReindexRepo and never touches s.mutationReceipts, so a scoped workspace_admin.reindex(paths=[<file>]) still leaves the failed receipt blocking detect — request 1 in the issue, and the half of your own issue comment that said "and when a direct reindex of that path succeeds". Your PR body doesn't claim it and the new barrier text is careful not to promise it, which I appreciate; I'd just say so explicitly in the PR so the issue stays open for it.

Same for the shape you actually hit: under sustained load where each successive ingest also fails, nothing here clears anything, because the resolve only runs on a success. That is fine as a scoped first step — it converts "blocked until retention" into "blocked until the next success", which is a real improvement — but it is not the rolling-lockout fix, and the issue should keep tracking retry-with-backoff and degrade-instead-of-refuse separately.

The message addition is good and I'd keep it as is.